[GLUTEN-5625][VL] Support window range frame #5626

WangGuangxin · 2024-05-06T18:32:57Z

What changes were proposed in this pull request?

Support window range frame by pre-project frame offset for non-SpecialFrameBoundary, based on the discussion facebookincubator/velox#7557

(Fixes: #5625)

How was this patch tested?

UT

github-actions · 2024-05-06T18:33:16Z

#5625

github-actions · 2024-05-06T18:33:30Z

Run Gluten Clickhouse CI

github-actions · 2024-05-06T18:44:17Z

Run Gluten Clickhouse CI

github-actions · 2024-05-06T18:47:29Z

Run Gluten Clickhouse CI

github-actions · 2024-05-06T18:59:10Z

Run Gluten Clickhouse CI

github-actions · 2024-05-06T19:03:55Z

Run Gluten Clickhouse CI

github-actions · 2024-05-06T20:05:42Z

Run Gluten Clickhouse CI

github-actions · 2024-05-06T23:53:49Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-05-07T01:58:12Z

cc @PHILO-HE @rui-mo

FelixYBW · 2024-05-07T04:23:46Z

@PHILO-HE is the PR offload SQL like below?

  sum(x) OVER (
    PARTITION BY id,sur
    ORDER BY date RANGE BETWEEN 6 preceding AND CURRENT ROW
  )

PHILO-HE · 2024-05-07T05:21:12Z

@PHILO-HE is the PR offload SQL like below?

  sum(x) OVER (
    PARTITION BY id,sur
    ORDER BY date RANGE BETWEEN 6 preceding AND CURRENT ROW
  )

@FelixYBW, yes, this pr can remove the fallback for such case.

FelixYBW · 2024-05-07T07:02:21Z

Let me do a test once merged. Thank you!

FelixYBW · 2024-05-07T17:07:52Z

@WangGuangxin can you help to resolve conflicts?

github-actions · 2024-05-08T00:16:01Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-05-08T11:03:39Z

@WangGuangxin can you help to resolve conflicts?

@FelixYBW done

PHILO-HE

Will spare some time to review in next week. Sorry for late response.

rui-mo

Thanks! Just added several comments.

rui-mo · 2024-05-20T02:30:16Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

+        return std::make_tuple(
+            core::WindowNode::BoundType::kFollowing,
+            exprConverter_->toVeloxExpr(boundType.following().ref(), inputType));
+      }


It seems the only difference of the two branches is the expression. Can we differentiate the expressions only and reuse the rest of code?

rui-mo · 2024-05-20T02:30:28Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

+        return std::make_tuple(
+            core::WindowNode::BoundType::kPreceding,
+            exprConverter_->toVeloxExpr(boundType.preceding().ref(), inputType));
+      }


done.
I tried to reuse the logic here, but since the type of boundType.preceding() and boundType.following() are different, so I have to pass the offset and ref instead of the expression itself

rui-mo · 2024-05-20T02:31:00Z

gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java

-      String upperBound,
-      String lowerBound,
+      org.apache.spark.sql.catalyst.expressions.Expression upperBound,
+      org.apache.spark.sql.catalyst.expressions.Expression lowerBound,


Can we import org.apache.spark.sql.catalyst.expressions.Expression and use it here?

The Expression is conflict with io.substrait.proto.Expression which alreay imported in this class, and Java doen't have the alias grammer like scala

rui-mo · 2024-05-20T02:32:31Z

gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java

+            if (offset < 0) {
+              Expression.WindowFunction.Bound.Preceding.Builder offsetPrecedingBuilder =
+                  Expression.WindowFunction.Bound.Preceding.newBuilder();
+              offsetPrecedingBuilder.setOffset(0 - offset);


Do we need -offset?

No need. In fact, this branch is the existing logic

rui-mo · 2024-05-20T02:34:28Z

gluten-core/src/main/resources/substrait/proto/substrait/algebra.proto

+
+          // the reference to pre-project range frame boundary.
+          Expression ref = 2;
+        }


Would you document this change to Substrait in https://github.com/apache/incubator-gluten/blob/main/docs/developers/SubstraitModifications.md?

github-actions · 2024-05-23T00:17:05Z

Run Gluten Clickhouse CI

github-actions · 2024-05-23T05:14:29Z

Run Gluten Clickhouse CI

github-actions · 2024-05-23T05:25:26Z

Run Gluten Clickhouse CI

github-actions · 2024-05-28T00:18:03Z

Run Gluten Clickhouse CI

github-actions · 2024-05-28T01:56:24Z

Run Gluten Clickhouse CI

PHILO-HE

Thanks for your great work! Just posted some comments. Please check whether it makes sense.

PHILO-HE · 2024-05-28T07:05:54Z

gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java

+            throw new UnsupportedOperationException(
+                "Unsupported Window Function Frame Type:" + boundType);
+          }
+        } else if (boundType.foldable()) {


This if branch is just for ClickHouse backend? It would be nice to leave some comments.

For clickhouse backend and velox backend with ROW frame. Comments added

PHILO-HE · 2024-05-28T09:02:10Z

gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java

+            }
+          } catch (NumberFormatException e) {
+            throw new UnsupportedOperationException(
+                "Unsupported Window Function Frame Type:" + boundType);


Do we need to add statements to catch this exception? I think Spark parser has already checked the validity of foldable bound (e.g., 100a is not valid). The catching looks unnecessary.

PHILO-HE · 2024-05-28T09:14:32Z

gluten-core/src/main/scala/org/apache/gluten/utils/PullOutProjectHelper.scala

+      case swf: SpecifiedWindowFrame if needPreComputeRangeFrame(swf) =>
+        // This is guaranteed by Spark, but we still check it here
+        if (orderSpecs.size != 1) {
+          throw new GlutenNotSupportException(


Let's use GlutenException. Generally GlutenNotSupportException is caught in fallback handling for some limitation cases. But for this case, I think we can directly let a runtime exception happen.

PHILO-HE · 2024-05-28T09:31:26Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

      return std::make_tuple(
          core::WindowNode::BoundType::kFollowing,
-          std::make_shared<core::ConstantTypedExpr>(BIGINT(), variant(boundType.following().offset())));
+          specifiedBound(following.has_offset(), following.offset(), following.ref()));


Can has_offset be true for Velox backend after the java side handling?

It's reused both for ROW and RANGE frame, so offset is used for ROW frame here.

github-actions · 2024-05-29T15:26:04Z

Run Gluten Clickhouse CI

github-actions · 2024-05-29T15:47:24Z

Run Gluten Clickhouse CI

PHILO-HE

Two minor comments. We can merge it after CI is fixed. Thanks!

PHILO-HE · 2024-05-30T00:46:48Z

gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java

+              offsetFollowingBuilder.setOffset(offset);
+              builder.setFollowing(offsetFollowingBuilder.build());
+            }
+          } catch (NumberFormatException e) {


Looks we can also remove this try/catch.

PHILO-HE · 2024-05-30T00:48:34Z

gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java

          }
-        } catch (NumberFormatException e) {
+        } else {
          throw new UnsupportedOperationException(
              "Unsupported Window Function Frame Type:" + boundType);


Not frame type, but bound type, please help fix it in your pr.

"Unsupported Window Function Frame Bound Type: " + boundType

github-actions · 2024-05-30T13:13:11Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-06-04T08:21:59Z

@PHILO-HE @rui-mo Can you please review it again?

PHILO-HE · 2024-06-04T08:37:27Z

@PHILO-HE @rui-mo Can you please review it again?

@WangGuangxin, the pr looks good to me. Could you fix the CI failure? Looks relevant to this pr.

github-actions · 2024-06-04T08:46:19Z

Run Gluten Clickhouse CI

PHILO-HE · 2024-06-04T08:48:56Z

@WangGuangxin, please also rebase the code. Thanks!

github-actions · 2024-06-04T08:52:32Z

Run Gluten Clickhouse CI

ulysses-you · 2024-06-04T09:16:30Z

gluten-core/src/main/scala/org/apache/gluten/utils/PullOutProjectHelper.scala

+        val a = expressionMap
+          .getOrElseUpdate(
+            bound.canonicalized,
+            Alias(Add(orderSpec.child, bound), generatePreAliasName)())


Do we really support reuse the reference for the same bound literal ? What happens if the two window expressions have the same frame but requires different ordering column.

sum(x) OVER ( PARTITION BY c1 ORDER BY c2 RANGE BETWEEN 1 preceding AND CURRENT ROW ), sum(x) OVER ( PARTITION BY c1 ORDER BY c3 RANGE BETWEEN 1 preceding AND CURRENT ROW )

Do we really support reuse the reference for the same bound literal ? What happens if the two window expressions have the same frame but requires different ordering column.

sum(x) OVER ( PARTITION BY c1 ORDER BY c2 RANGE BETWEEN 1 preceding AND CURRENT ROW ), sum(x) OVER ( PARTITION BY c1 ORDER BY c3 RANGE BETWEEN 1 preceding AND CURRENT ROW )

let me check

The frame bouding is a reference, which will be pre-project before window operator. And the sort/partition are calculated in window operator. So it's ok to reuse it.
I add a UT for this.

The new added UT succeed in spark3.2, but failed in spark3.4/3.5. I'll fix it

Sorry for late response, all UT passed now, the failed one seems unrelated. Please help review again @PHILO-HE @ulysses-you

The reason why it was failed is because sort column l_discount is a decimal, and when we do Add(sort_col, offset), the precision of decimal type will change, so that the data type check failed in Spark (https://github.com/apache/spark/blob/82a84ede6a47232fe3af86672ceea97f703b3e8a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala#L83)

In fact, we only support ByteType | ShortType | IntegerType | LongType | DateType by design, which is checked in https://github.com/apache/incubator-gluten/pull/5626/files#diff-68925b6a5ee67a942a976971ae6727a45a0b166d200d8d983454169e7c3bd1f6R316, but even when WindowExec fallback by this check, the PullOutPreProjection rule will still be executed.

github-actions · 2024-06-05T07:44:25Z

Run Gluten Clickhouse CI

github-actions · 2024-06-11T16:18:35Z

Run Gluten Clickhouse CI

github-actions · 2024-06-11T16:19:39Z

Run Gluten Clickhouse CI

github-actions · 2024-06-11T16:23:12Z

Run Gluten Clickhouse CI

ulysses-you

thank you @WangGuangxin

WangGuangxin force-pushed the feat_window_range_frame branch from f5d363b to 0484902 Compare May 6, 2024 23:53

WangGuangxin force-pushed the feat_window_range_frame branch from 0484902 to 7d570fb Compare May 8, 2024 00:15

PHILO-HE reviewed May 11, 2024

View reviewed changes

rui-mo reviewed May 20, 2024

View reviewed changes

WangGuangxin force-pushed the feat_window_range_frame branch from 7d570fb to d5195af Compare May 23, 2024 00:16

WangGuangxin force-pushed the feat_window_range_frame branch from c2b472f to 7947976 Compare May 28, 2024 00:17

PHILO-HE reviewed May 28, 2024

View reviewed changes

PHILO-HE reviewed May 30, 2024

View reviewed changes

WangGuangxin force-pushed the feat_window_range_frame branch from 8e87f6e to 87a4692 Compare June 4, 2024 08:51

ulysses-you reviewed Jun 4, 2024

View reviewed changes

WangGuangxin force-pushed the feat_window_range_frame branch from 2680b9f to e726284 Compare June 11, 2024 16:19

WangGuangxin added 9 commits June 12, 2024 00:21

Support range frame

eb736c7

address comments

dcc47b6

fix style

e8778be

address comments

c540642

fix

58ffa22

address comments

b2be152

fix typo

695b875

add more ut

325b710

fix decimal

339751a

WangGuangxin force-pushed the feat_window_range_frame branch from e726284 to 339751a Compare June 11, 2024 16:22

ulysses-you approved these changes Jun 12, 2024

View reviewed changes

ulysses-you merged commit 7e217cf into apache:main Jun 12, 2024
39 of 40 checks passed

[GLUTEN-5625][VL] Support window range frame #5626

[GLUTEN-5625][VL] Support window range frame #5626

Conversation

WangGuangxin commented May 6, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

WangGuangxin commented May 7, 2024

FelixYBW commented May 7, 2024

PHILO-HE commented May 7, 2024

FelixYBW commented May 7, 2024

FelixYBW commented May 7, 2024

github-actions bot commented May 8, 2024

WangGuangxin commented May 8, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 28, 2024

github-actions bot commented May 28, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024

github-actions bot commented May 29, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2024

WangGuangxin commented Jun 4, 2024

PHILO-HE commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

PHILO-HE commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 11, 2024

github-actions bot commented Jun 11, 2024

github-actions bot commented Jun 11, 2024

ulysses-you left a comment

Choose a reason for hiding this comment

WangGuangxin commented May 6, 2024 •

edited

Loading